-
Notifications
You must be signed in to change notification settings - Fork 40
Rewrite in functional style #268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I wanted to look at this today but I didn’t find the time for it. I am definitely open for refactoring and I’ll have a closer look after the weekend. |
Not quite ready to merge, but complete enough to be worth a review. |
Thanks for taking the time to write this PR. I mostly decided to go the class way because Sentry's class based too, but I like your approach as well. Let me know when it's ready to be merged! |
I am going to be battle-testing this for a few more days with the current feature set. How should I best document the changes? How about an |
An |
… object BREAKING CHANGE: rename response key to fetchResult add operationName key add url key
Did some more testing with it, everything is working fine. I rewrote the commit messages and added both changelog and upgrade guide. Everything should be ready to merge :) |
Cheers. Merged & released. |
Thank you. Next up, support for spans :) |
During todays attempt to implement #265, i found it increasingly difficult to
In my experience, plain objects and (mostly) pure functions lead to simpler code that is easier to work with, less stateful and less error prone. What do you think about this approach?
This PR is just a draft as of now. If you don't like it, that is totally fine, i can also maintain it in my fork. Perhaps, we can find common ground and continue working on this together. Next steps for me would be to add spans and some performance tracing.
In any case thank you for your previous work on this, i found the general structure and options to really make sense.